Add DASH HA session API design. #532
Conversation
| #ifndef _DASH_STAGE_HA_P4_ | ||
| #define _DASH_STAGE_HA_P4_ | ||
|
|
||
| control ha_stage(inout headers_t hdr, |
There was a problem hiding this comment.
@r12f, as discussed can we add a capability attribute for DPU scope versus ENI scope HA.
There was a problem hiding this comment.
Hi Mukesh, sure thing! And as we chatted, it will be better to add it in another PR.
The DPU HA capability will make more sense to be added along with the DPU API or a dedicated one, if needed. Putting the DPU HA capability into ENI HA API will be really weird...
There was a problem hiding this comment.
Actually, let me get some capacities in there, we can probably model this in a generic way.
There was a problem hiding this comment.
Hi @mukeshmv , I have added the capabilities in the PR :D. Feel free to help take a look!
| typedef enum _sai_ha_set_event_t | ||
| { | ||
| /** Any HA set state is changed, such as data plane channel goes down. */ | ||
| SAI_HA_SET_STATE_CHANGED, |
There was a problem hiding this comment.
This enum should have the possible states to remove the need to read them from the attribute.
There was a problem hiding this comment.
ah, yea. i am mostly following what other SAI events is doing, but please correct me if my understanding is not right.
yes, the event enum doesn't contain the detailed state, but we don't need to read them from HA set attribute using query. the reason is the changed attributes will be reported as attr_count and attr in the sai_ha_set_event_data, hence we can directly fetch them there.
I found this approach being a repetitive pattern in existing SAI APIs, so i am mostly following this pattern. It does simplify the event data definition and won't cause state explosion on the enum value.
There was a problem hiding this comment.
Notification data doesn't carry attributes but the type that attribute has. See https://github.com/opencomputeproject/SAI/blob/master/inc/saiport.h#L90C5-L90C28 for reference.
There was a problem hiding this comment.
yea, i know ACL and Port notification is doing it, but it might not be extendable, because we need to change the API whenever a new field needs to be added.
on the otherhand, FDB, packet event are using attributes directly, which seems to be a more flexiable approach, but i guess the thing I am missing here is the bar, FDB needs to report all its attributes, hence designed like that.
so no problem at all, let me get the event updated.
There was a problem hiding this comment.
FDB attribute is used to indicate a bridge ID and an entry type - https://github.com/sonic-net/sonic-swss/blob/master/orchagent/fdborch.cpp#L1054, not the state. State is passed as part of the notification data separately. Why would it be harder to extend if both are using the same type?
| _In_ const sai_ha_set_event_data_t *ha_set_event_data); | ||
| ``` | ||
|
|
||
| #### 4.6.2. HA scope event notifications |
There was a problem hiding this comment.
Seems redundant, HA scope does not define liveliness API for this event, it's in the HA_SET.
There was a problem hiding this comment.
I see, the HA scope state change event is used for reporting the HA state changes. For example, whenever we call SAI API to update the HA role to "Active". It might take some time for the implementation to process the request and really switch to active. Then whenever it is completed, we can send a HA scope event with the HA role set to "Active" in its event data.
This also helps the case where DPU decides to drive the HA state machine without using NPU (HLD here). Whenever the HA role is changed by DPU, it will be reported using this event notification as well.
There was a problem hiding this comment.
Setting HA scope role is synchronous. the set_attribute call should not return until it is completed.
There was a problem hiding this comment.
yea, it could be synchronous for the first case. But for the second case, this notification will be required, otherwise we won't be able to know the state being changed.
| typedef enum _sai_ha_set_event_t | ||
| { | ||
| /** Any HA set state is changed, such as data plane channel goes down. */ | ||
| SAI_HA_SET_STATE_CHANGED, |
There was a problem hiding this comment.
Notification data doesn't carry attributes but the type that attribute has. See https://github.com/opencomputeproject/SAI/blob/master/inc/saiport.h#L90C5-L90C28 for reference.
| _In_ const sai_ha_set_event_data_t *ha_set_event_data); | ||
| ``` | ||
|
|
||
| #### 4.6.2. HA scope event notifications |
There was a problem hiding this comment.
Setting HA scope role is synchronous. the set_attribute call should not return until it is completed.
|
Woot! Thanks @mukeshmv and @marian-pritsak for reviewing! I am merging it now. |
This change is made to add HA session API HLD and P4 changes for SAI API generation.
To describe how the HA session API works, the HLD in this change contains:
The change also contains the P4 code that generates the SAI APIs. And the generated APIs can be previewed in this branch here: https://github.com/r12f/SAI/tree/user/r12f/ha-api/experimental.